-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat(autocomplete): Add promise support to md-item-text #2710
Conversation
Without giving it too much thought, I suspect that this will lead to unexpected results if a promise gets resolved after the next call to |
@gkalpak I think your right.
I made |
@ThomasBurleson Any chance this will be merged for 0.10.0? I can get it working 0.9.7 today if you want to try and merge it. I've had about 20 people using it for a while now. |
@epelc - working on this now. |
@ThomasBurleson Cool! Do you want me to get it working with the current master? There were several additions and fixes for auto complete since I first created this but it shouldn't take too long to update. |
@epelc - yes. plz rebase with master. |
727b1c1
to
861c426
Compare
Everything works besides the tests now. I'll get them running first thing tomorrow morning. I was having trouble getting karma to run on this machine. Also I found #3546 while upgrading everything. |
This reverts commit 727b1c1.
861c426
to
a7151b2
Compare
@robertmesserle Can you take a quick look? |
@epelc - you did not provide a unit test for this promise-based internal feature. /**
* Returns the display value for an item.
* @param item
* @returns {*}
*/
function getDisplayValue (item) {
return $q.when( getItemText(item) || item );
/**
* Getter function to invoke user-defined expression (in the directive)
* to convert your object to a single string.
*/
function getItemText(item) {
return (item && $scope.itemText) ? $scope.itemText(getItemAsNameVal(item)) : null;
}
} where the I will merge this PR. Can you provide another with unit tests? |
@ThomasBurleson I'm trying to make a unit test for it now. I can just add it to this if you want. |
Oops. |
It's fine I'll make a new pr. Does this look ok? I haven't written many karma tests before. I have this right after the basic functionality tests here. Would you organize it this way or make a new
|
@ThomasBurleson ping Can you look at the above. I forgot to mention you. |
This adds promise support for autocompletes
md-item-text
option. It makes use of$q.when()
ingetDisplayValue()
to convert all functions to promises. Because of this I had to change several things around to make the rest of the autocomplete controller work with promises when it usedgetDisplayValue()
.I tried running the karma tests and got a few errors related to
md-autocomplete
but I also got errors when I ran it on the just released0.9.0
. I checked several things on the docs auto complete demo locally with this build and also in my app and it functions the same as the official docs so I don't think I broke anything.This should close #2462 If you want to see a use case see that issue I explained how it's needed for the google places api. But in general this is needed anytime you need to fill in extra data after you select something.
@ThomasBurleson Can you take a look at this please?